Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add MOI methods: delete and modify #584

Merged
merged 5 commits into from
Aug 13, 2021
Merged

Add MOI methods: delete and modify #584

merged 5 commits into from
Aug 13, 2021

Conversation

laradicp
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Aug 12, 2021 •
edited
Loading

Codecov Report

Merging #584 (da2a964) into master (921592d) will decrease coverage by 0.17%.
The diff coverage is 68.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #584      +/-   ##
==========================================
- Coverage   86.82%   86.65%   -0.18%     
==========================================
  Files          47       47              
  Lines        4752     4796      +44     
==========================================
+ Hits         4126     4156      +30     
- Misses        626      640      +14
Impacted Files Coverage Δ
src/MOIwrapper.jl 84.26% <67.56%> (-1.52%) ⬇️
src/MathProg/varconstr.jl 86.28% <71.42%> (-0.62%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 921592d...da2a964. Read the comment docs.

guimarqu
guimarqu reviewed Aug 12, 2021
View reviewed changes
src/MOIwrapper.jl
push!(varids, varid)
end
for varid in varids
coefmatrix[constrid, varid] = 0.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you do :

for (varid, _) in @view coefmatrix[constrid, :]
        coefmatrix[constrid, varid] = 0.0
end

Copy link
Contributor

@guimarqu guimarqu Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it enters in an undefined behavior because the size of the array that stores the coefficient matrix may change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, I got an error for accessing an invalid index.

@laradicp laradicp marked this pull request as ready for review August 13, 2021 03:55
@laradicp laradicp requested a review from guimarqu August 13, 2021 03:55
guimarqu
guimarqu reviewed Aug 13, 2021
View reviewed changes
Copy link
Contributor

@guimarqu guimarqu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to create methods to delete variable and constraint in MathProg.

So we would have :

    delete!(formulation, var)
    delete!(formulation, varid)
    delete!(formulation, constr)
    delete!(formulation, constrid)

guimarqu
guimarqu reviewed Aug 13, 2021
View reviewed changes
src/MOIwrapper.jl Outdated Show resolved Hide resolved
src/MOIwrapper.jl Outdated Show resolved Hide resolved
src/MOIwrapper.jl Outdated Show resolved Hide resolved
guimarqu
guimarqu reviewed Aug 13, 2021
View reviewed changes
src/MathProg/varconstr.jl

function Base.delete!(form::Formulation, constrid::ConstrId)
delete!(form.buffer.constr_buffer.added, constrid)
delete!(form.manager.constrs, constrid)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
delete!(form.manager.constrs, constrid)
delete!(form.manager.constrs, constrid)
return

src/MOIwrapper.jl
Comment on lines +261 to +263
for (varid, _) in @view coefmatrix[constrid, :]
push!(varids, varid)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should change the coefficients in MathProg.delete!.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah indeed

@guimarqu
Copy link
Contributor

I need these changes to fix #583 so I merge into master.

@guimarqu guimarqu merged commit 3c64445 into master Aug 13, 2021
@guimarqu guimarqu deleted the delete_modify branch August 13, 2021 20:22
laradicp added a commit that referenced this pull request Aug 13, 2021
@laradicp
apply suggestions from PR #584
726fd7d
guimarqu added a commit that referenced this pull request Aug 13, 2021
@laradicp @guimarqu
Apply suggestions from PR #584 review (#589)
b7caebc 
* add delete and modify methods for constraints

* add delete and modify methods for variables

* fix merge commit

* add delete! and use proper get and set methods

* apply suggestions from PR #584

Co-authored-by: Guillaume Marques <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guimarqu guimarqu guimarqu left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@laradicp @guimarqu